ostree/trivial-httpd: Fix --autoexit with --daemonize and --log-file

When --autoexit is used with --daemonize and --log-file, the program
never exits when the root directory is deleted. I believe what happens
is that g_file_new_for_path triggers the glib worker context to be
started to talk to GVfs. Once the program forks, the parent exits and
the thread iterating the worker context is gone. The file monitor then
never receives any events because the inotify helper also runs from the
worker context.

Move the fork earlier just after parsing and validating the command line
arguments. In order to handle setup errors in the child, a pipe is
opened and the parents waits until the child writes a status byte to it.
If the byte is 0, the parent considers the child setup successful and
exits and the child carries on as a daemon. Notably, the child doesn't
reopen stderr to /dev/null until after this so that it can send error
messages there.

Fixes: #1941

diff --git a/src/ostree/ostree-trivial-httpd.c b/src/ostree/ostree-trivial-httpd.c
index 5d3a004ee420e8da8fd02494d8bc753abea777ac..e5dfc320f859f613b9f6015fdc625fde6c263d13 100644 (file)
--- a/src/ostree/ostree-trivial-httpd.c
+++ b/src/ostree/ostree-trivial-httpd.c
@@ -507,6 +507,7 @@ run (int argc, char **argv, GCancellable *cancellable, GError **error)
   const char *dirpath;
   OtTrivialHttpd appstruct = { 0, };
   OtTrivialHttpd *app = &appstruct;
+  int pipefd[2] = { -1, -1 };
   glnx_unref_object SoupServer *server = NULL;
   g_autoptr(GFileMonitor) dirmon = NULL;
 
@@ -540,17 +541,86 @@ run (int argc, char **argv, GCancellable *cancellable, GError **error)
       goto out;
     }
 
-  if (opt_log)
+  if (opt_daemonize && (g_strcmp0 (opt_log, "-") == 0))
     {
-      GOutputStream *stream = NULL;
+      ot_util_usage_error (context, "Cannot use --log-file=- and --daemonize at the same time", error);
+      goto out;
+    }
 
-      if (g_strcmp0 (opt_log, "-") == 0)
+  /* Fork early before glib sets up its worker context and thread since they'll
+   * be gone once the parent exits. The parent waits on a pipe with the child to
+   * handle setup errors. The child writes a 0 when setup is successful and a 1
+   * otherwise.
+   */
+  if (opt_daemonize)
+    {
+      if (pipe (pipefd) == -1)
+        {
+          glnx_set_error_from_errno (error);
+          goto out;
+        }
+
+      pid_t pid = fork();
+      if (pid == -1)
+        {
+          glnx_set_error_from_errno (error);
+          goto out;
+        }
+      else if (pid > 0)
+        {
+          /* Parent, read the child status from the pipe */
+          glnx_close_fd (&pipefd[1]);
+          guint8 buf;
+          int res = TEMP_FAILURE_RETRY (read (pipefd[0], &buf, 1));
+          if (res < 0)
+            {
+              glnx_set_error_from_errno (error);
+              goto out;
+            }
+          else if (res == 0)
+            {
+              g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                           "Child process closed pipe without writing status");
+              goto out;
+            }
+
+          g_debug ("Read %u from child", buf);
+          if (buf > 0)
+            {
+              g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                           "Child process failed during setup");
+              goto out;
+            }
+          glnx_close_fd (&pipefd[0]);
+
+          ret = TRUE;
+          goto out;
+        }
+
+      /* Child, continue */
+      glnx_close_fd (&pipefd[0]);
+    }
+  else
+    {
+      /* Since we're used for testing purposes, let's just do this by
+       * default.  This ensures we exit when our parent does.
+       */
+      if (prctl (PR_SET_PDEATHSIG, SIGTERM) != 0)
         {
-          if (opt_daemonize)
+          if (errno != ENOSYS)
             {
-              ot_util_usage_error (context, "Cannot use --log-file=- and --daemonize at the same time", error);
+              glnx_set_error_from_errno (error);
               goto out;
             }
+        }
+    }
+
+  if (opt_log)
+    {
+      GOutputStream *stream = NULL;
+
+      if (g_strcmp0 (opt_log, "-") == 0)
+        {
           stream = G_OUTPUT_STREAM (g_unix_output_stream_new (STDOUT_FILENO, FALSE));
         }
       else
@@ -625,45 +695,39 @@ run (int argc, char **argv, GCancellable *cancellable, GError **error)
 #if !SOUP_CHECK_VERSION(2, 48, 0)
   soup_server_run_async (server);
 #endif
-  
+
   if (opt_daemonize)
     {
-      pid_t pid = fork();
-      if (pid == -1)
+      /* Write back a 0 to the pipe to indicate setup was successful. */
+      guint8 buf = 0;
+      g_debug ("Writing %u to parent", buf);
+      if (TEMP_FAILURE_RETRY (write (pipefd[1], &buf, 1)) == -1)
         {
-          int errsv = errno;
-          g_set_error_literal (error, G_IO_ERROR, g_io_error_from_errno (errsv),
-                               g_strerror (errsv));
+          glnx_set_error_from_errno (error);
           goto out;
         }
-      else if (pid > 0)
+      glnx_close_fd (&pipefd[1]);
+
+      if (setsid () < 0)
         {
-          ret = TRUE;
+          glnx_set_prefix_error_from_errno (error, "%s", "setsid: ");
           goto out;
         }
-      /* Child, continue */
-      if (setsid () < 0)
-        err (1, "setsid");
       /* Daemonising: close stdout/stderr so $() et al work on us */
       if (freopen("/dev/null", "r", stdin) == NULL)
-        err (1, "freopen");
+        {
+          glnx_set_prefix_error_from_errno (error, "%s", "freopen: ");
+          goto out;
+        }
       if (freopen("/dev/null", "w", stdout) == NULL)
-        err (1, "freopen");
+        {
+          glnx_set_prefix_error_from_errno (error, "%s", "freopen: ");
+          goto out;
+        }
       if (freopen("/dev/null", "w", stderr) == NULL)
-        err (1, "freopen");
-    }
-  else
-    {
-      /* Since we're used for testing purposes, let's just do this by
-       * default.  This ensures we exit when our parent does.
-       */
-      if (prctl (PR_SET_PDEATHSIG, SIGTERM) != 0)
         {
-          if (errno != ENOSYS)
-            {
-              glnx_set_error_from_errno (error);
-              goto out;
-            }
+          glnx_set_prefix_error_from_errno (error, "%s", "freopen: ");
+          goto out;
         }
     }
 
@@ -699,6 +763,21 @@ run (int argc, char **argv, GCancellable *cancellable, GError **error)
 
   ret = TRUE;
  out:
+  if (pipefd[0] >= 0)
+    {
+      /* Read end in the parent. This should only be open on errors. */
+      g_assert_false (ret);
+      glnx_close_fd (&pipefd[0]);
+    }
+  if (pipefd[1] >= 0)
+    {
+      /* Write end in the child. This should only be open on errors. */
+      g_assert_false (ret);
+      guint8 buf = 1;
+      g_debug ("Writing %u to parent", buf);
+      (void) TEMP_FAILURE_RETRY (write (pipefd[1], &buf, 1));
+      glnx_close_fd (&pipefd[1]);
+    }
   if (app->root_dfd != -1)
     (void) close (app->root_dfd);
   g_clear_object (&app->log);